Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

algorithm-deps-install #1081

Merged
merged 7 commits into from
Jan 5, 2021
Merged

algorithm-deps-install #1081

merged 7 commits into from
Jan 5, 2021

Conversation

yehiyam
Copy link
Contributor

@yehiyam yehiyam commented Jan 5, 2021

This change is Reviewable

@yehiyam
Copy link
Contributor Author

yehiyam commented Jan 5, 2021

/deploy

@hkube-ci hkube-ci temporarily deployed to dev January 5, 2021 08:54 Inactive
@yehiyam
Copy link
Contributor Author

yehiyam commented Jan 5, 2021

/deploy

@hkube-ci hkube-ci temporarily deployed to dev January 5, 2021 10:18 Inactive
nassiharel
nassiharel previously approved these changes Jan 5, 2021
Copy link
Contributor

@nassiharel nassiharel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 18 of 20 files at r1, 2 of 2 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yehiyam)


core/api-server/lib/service/builds.js, line 225 at r4 (raw file):

dependencyInstallCmd 

You can add a test for that, just copy a test from tests/algorithms-store.js with:
"a build was triggered due to"

Copy link
Contributor Author

@yehiyam yehiyam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 20 of 21 files reviewed, 1 unresolved discussion (waiting on @nassiharel)


core/api-server/lib/service/builds.js, line 225 at r4 (raw file):

Previously, NassiHarel (Nassi Harel) wrote…
dependencyInstallCmd 

You can add a test for that, just copy a test from tests/algorithms-store.js with:
"a build was triggered due to"

Done.

@yehiyam yehiyam enabled auto-merge (squash) January 5, 2021 12:21
@yehiyam yehiyam requested a review from nassiharel January 5, 2021 12:21
Copy link
Contributor

@nassiharel nassiharel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r5.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@yehiyam yehiyam merged commit faf983d into master Jan 5, 2021
@yehiyam yehiyam deleted the algorithm-deps-install branch January 5, 2021 12:31
hkube-ci pushed a commit that referenced this pull request Jan 5, 2021
* add dependencyInstallCmd option to algorithm
add python support

* add nodejs support

* fix install deps script permission

* add test .... bump version [skip ci]
hkube-ci pushed a commit that referenced this pull request Jan 5, 2021
* add dependencyInstallCmd option to algorithm
add python support

* add nodejs support

* fix install deps script permission

* add test .... bump version [skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants